Conversation
…le staves, multiple parts
…gle stave, different notes
…gle stave, repeat with endings
…gle stave, multiple systems
…ckwards formatting
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issue #281 by updating the hint generation logic and improving unit tests through new describer patterns. Key changes include:
- Updating expected human-readable hint outputs in unit tests.
- Propagating the new ElementDescriber and HintDescriber through playback components.
- Renaming MeasureSequenceIterator to LegacyMeasureSequenceIterator and updating its usage.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/playback/timeline.test.ts | Updated expected hint output strings to include ElementDescriber formatting changes |
| tests/unit/playback/measuresequenceiterator.test.ts | Renamed MeasureSequenceIterator to LegacyMeasureSequenceIterator and updated its usage |
| tests/unit/playback/lazycursorstatehintprovider.test.ts | Updated tests for hints to use new hint describer and human-readable outputs |
| tests/unit/playback/defaultcursorframe.test.ts | Passed ElementDescriber instances to DefaultCursorFrame.create |
| src/playback/types.ts | Added toHumanReadable method on CursorStateHintProvider |
| src/playback/timeline.ts | Integrated ElementDescriber into TimelineFactory and TimelineDescriber |
| src/playback/legacymeasuresequenceiterator.ts | Renamed the iterator class from MeasureSequenceIterator to LegacyMeasureSequenceIterator |
| src/playback/lazycursorstatehintprovider.ts | Added toHumanReadable implementation and updated note comparison conditions |
| src/playback/hintdescriber.ts | Introduced new HintDescriber class |
| src/playback/elementdescriber.ts | Added ElementDescriber to produce consistent human-readable descriptions |
| src/playback/defaultcursorframe.ts | Updated factory and describer usage to include ElementDescriber |
| src/playback/cursor.ts | Updated Cursor creation and iteration to pass ElementDescriber |
| src/elements/score.ts | Integrated ElementDescriber in creating frames and cursors |
Comments suppressed due to low confidence (2)
src/playback/legacymeasuresequenceiterator.ts:13
- [nitpick] Renaming MeasureSequenceIterator to LegacyMeasureSequenceIterator clarifies legacy behavior; consider adding documentation to detail the differences from the current implementation.
export class LegacyMeasureSequenceIterator<T extends Measure> implements Iterable<number> {
src/playback/lazycursorstatehintprovider.ts:78
- Adding the check for previousNote !== currentNote improves safety by avoiding self-comparisons; please ensure that this change is in line with the intended behavior in hint generation.
if (previousNote && previousNote !== currentNote && !previousNote.sharesACurveWith(currentNote)) {
Collaborator
Author
|
@rvilarl, I'm going to release a vexml patch with this PR so that you're unblocked. There may be still issues with repeats and chords. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes #281. It will be released in 0.1.8.